Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed SAML logout for ADFS #2902

Merged
merged 3 commits into from
Oct 23, 2021
Merged

Conversation

theodor-franke
Copy link

@theodor-franke theodor-franke commented Aug 30, 2021

This PR. adds some options to the .env file for configuring the logout with SAML. It also sets the correct NameId in the logout request.

This PR enables:

  • Signed Logout SAML-Assertions (default is disabled)
  • Sets the correct email address in the logout assertion
  • Enables the encoding of lowercase URLs (default is disabled)
  • some little tweaks in the logout process so that it works with ADFS (theses changes must be enabled in the .env file, without any changes in the .env file the logout process is not altered.)

@theodor-franke
Copy link
Author

In ADFS you need to have a Rule like this:

c:[Type == "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/emailaddress"]
 => issue(Type = "http://schemas.xmlsoap.org/ws/2005/05/identity/claims/nameidentifier", Issuer = c.Issuer, OriginalIssuer = c.OriginalIssuer, Value = c.Value, ValueType = c.ValueType, Properties["http://schemas.xmlsoap.org/ws/2005/05/identity/claimproperties/format"] = "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress", Properties["http://schemas.xmlsoap.org/ws/2005/05/identity/claimproperties/spnamequalifier"] = "https://<FQDN_OF_BOOKSTACK>/saml2/metadata");

and in your .env file you need to add the following lines:

SAML2_SP_CERTIFICATE=MIIFtT... (your cert)
SAML2_SP_PRIVATEKEY=MIIJRAIBADANBgkqhkiG... (your private key)
SAML2_SP_NAME_ID_Format=urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress
SAML2_SP_NAME_ID_SP_NAME_QUALIFIER=https://<FQDN_OF_BOOKSTACK>/saml2/metadata
SAML2_RETRIEVE_PARAMETERS_FROM_SERVER=true
SAML2_LOGOUT_REQUEST_SIGNED=true
SAML2_LOGOUT_RESPONSE_SIGNED=true
SAML2_LOWERCASE_URLENCODING=true

@ssddanbrown
Copy link
Member

Thanks for offering this @theodor-franke.

To be honest it'll probably take some time before I review this deeply, since I know it's going to consume about a day's worth of effort for me to go through each of the 8 added options to understand their functionality so I can assess the backwards compatibility and testing we'll need to add where possible.

At a first glance, This seems like a lot of new options just to fix ADFS SLS. Are all these changes required to make ADFS SLS work? Or could you describe exactly what was failing for ADFS SLS? If this goes beyond I'd prefer to keep the scope limited to just fixing the ADFS SLS issues unless there's significant demand/response to go outside of that.

In regards to the implementation, I have not functionally checked anything, but one thing I've seen from a quick look is that any env() usages outside of the Config/ directory will need to be altered to use the config system instead like the existing options set. Using env() outside this directory can cause issues, especially for those who cache their config for performance.

@theodor-franke
Copy link
Author

I only tried this on our coporate ADFS-Server (windows 2016 so I think its called ADFS4.0) and without these fixes the logout would not work.
The first important thing is the Certificate, ADFS requires the logout-assertion to be signed. The other important thing ist that the correct EntityID is set in this SAML-Assertion. I made all these Settings and not only one big USE_ADFS Setting because I think that other IDP may need these configs as well.

regarding the env() problem: If you think that this is a big problem I will try to find a solution for this. In my opinion this isnt a big deal because you will only ajust these ones and than (hopfully) never touch them again.

@ssddanbrown
Copy link
Member

Hi @theodor-franke,

Just been testing this after setting up a local ADFS environment.
One thing I've found troublesome is the SAML2_SP_NAME_ID_SP_NAME_QUALIFIER option. I have to specifically omit this in my environment otherwise ADFS reports the following error:

The SAML Single Logout request does not correspond to the logged-in session participant. 
Requestor: https://bookstack.local/saml2/metadata 
Request name identifier: Format: urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress, NameQualifier:  SPNameQualifier: https://bookstack.local/saml2/metadata, SPProvidedId:  
Logged-in session participants: 
Count: 1, [Issuer: https://bookstack.local/saml2/metadata, NameID: (Format: urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress, NameQualifier:  SPNameQualifier: , SPProvidedId: )]  

This request failed. 

User Action 
Verify that the claim provider trust or the relying party trust configuration is up to date. If the name identifier in the request is different from the name identifier in the session only by NameQualifier or SPNameQualifier, check and correct the name identifier policy issuance rule using the AD FS Management snap-in.

Within your environment, is there specific configuation that makes the SAML2_SP_NAME_ID_SP_NAME_QUALIFIER a requirement? Perhaps some specific issuance transform rules?

In addition, Was the SAML2_LOWERCASE_URLENCODING option definately a requirement for your environment? I don't seem to require this at all.


Notes to self

  • env() usage will need to change to config vars.
  • SLS requests now always provides email as nameId but the format is currently configurable. Probably better to hard-code this format but need to get confidence it won't break usage with existing IDPs.
  • The new cert .env var(s) should be tweaked to align with existing options.
  • The SAML2_RETRIEVE_PARAMETERS_FROM_SERVER and SAML2_LOWERCASE_URLENCODING (if needed) options are very specific to the toolkit naming. Ideally try to find better naming for these related to their usage to be library abstract where possible.
  • Should we just auto-set SAML_LOGOUT_*_SIGNED options if certificates are provided? Should we also set authnRequestsSigned to true in this case? Should be safe to do both if working since the certs are new options.

@theodor-franke
Copy link
Author

to resolve the Error you need to add the custom rule I posted earlier. (and dont forget to change the FQDN). About SAML2_LOWERCASE_URLENCODING iam not shure, but I thought it was important. I dont have access to the Environment I tested this with. But if it works fine on your site without this setting I will reach out to the Company i deployed this and test it there.

Thnaks for the work!

@ssddanbrown
Copy link
Member

@theodor-franke Thanks for the quick reply.

to resolve the Error you need to add the custom rule I posted earlier.

Ah, I see. Is there a reason for the spnamequalifier part to be defined in the rule? Does this functionally achieve something? To me it seems like we're having to add the static setting on the BookStack side because it's configured to be expected on the ADFS side, Is there a particular issue with instead simply not setting it on either side (Which works in my simplified setup). Sorry if that's used for something obvious but I'm not that well versed with ADFS or SAML2.

ssddanbrown added a commit that referenced this pull request Oct 23, 2021
- Migrated env usages to config.
- Removed potentially unneeded config options or auto-set signed options
  based upon provision of certificate.
- Aligned SP certificate env option naming with similar IDP option.

Tested via AFDS on windows server 2019. To test on other providers.
@ssddanbrown ssddanbrown merged commit 2e9ac21 into BookStackApp:master Oct 23, 2021
@ssddanbrown ssddanbrown added this to the Next Feature Release milestone Oct 23, 2021
@ssddanbrown
Copy link
Member

ssddanbrown commented Oct 23, 2021

Thanks again @theodor-franke. Now merged for next feature release.

Range of changes made in 98072ba. This streamlines a range of the options to limit impact and amount of options to manage, While tweaking env option names.

I have left the lowercaseUrlencoding as false, while ommiting the spnamequalifier as per my comment above. can always re-consider if deemed required after further feedback, but these were not needing as part of my testing so left out for now. The lowercaseUrlencoding setting could be set via the overrides option for now if needed.

Have manually tested these changes against the following:

  • ADFS (Windows Server 2019) (With SLS)
  • KeyCloak (With SLS)
  • Jumpcloud (Auth only, SLS not supported)
  • Okta (Auth only, SLS only supported via post)
  • SimpleSAMLphp (With SLS)

Each tested with pre-merge state going to post-merge state, ensuring no breakage post-merge.

@nomnombearclaw
Copy link

I've been trying to get SAML working with ADFS running on Windows Server 2019 and not having much luck. I've combed the documentation and still haven't had much luck. Since I'm running Windows Server 2019, are these new features/settings required? Does anyone have their working setup documented so I could go off something that is known to work? Any help would be appreciated.

@theodor-franke
Copy link
Author

theodor-franke commented May 27, 2022

I can't share my Documentation because I dont work any longer at the place where I installed this. However the docs are a great starting point (https://www.bookstackapp.com/docs/admin/saml2-auth/) and if you want you can write me when you have Problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants